Skip to content

[ubsan] Improve lowering of @llvm.allow.ubsan.check #119013

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Dec 8, 2024
Merged

[ubsan] Improve lowering of @llvm.allow.ubsan.check #119013

merged 3 commits into from
Dec 8, 2024
+8 −9

Conversation

@vitalybuka
Copy link
Collaborator

@vitalybuka vitalybuka commented Dec 6, 2024

This fix the case, when single hot inlined callsite, prevent
checks for all other. This helps to reduce number of removed checks up to 50% (deppedes on cutoff-hot value) .

ScalarOptimizerLateEPCallback was happening during
CGSCC walk, after each inlining, but this is effectively
after inlining.

Example, order in comments:

static void overflow() {
  // 1. Inline get/set if possible
  // 2. Simplify
  // 3. LowerAllowCheckPass
  set(get() + get());
}

void test() {
  // 4. Inline
  // 5. Nothing for LowerAllowCheckPass
  overflow();
}

With this patch it will look like:

static void overflow() {
  // 1. Inline get/set if possible
  // 2. Simplify
  set(get() + get());
}

void test() {
  // 3. Inline
  // 4. Simplify
  overflow();
}

// Later, after inliner CGSCC walk complete:
// 5. LowerAllowCheckPass for `overflow`
// 6. LowerAllowCheckPass for `test`

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. labels Dec 6, 2024
@vitalybuka vitalybuka requested a review from kstoimenov December 6, 2024 19:18
@llvmbot
Copy link
Member

llvmbot commented Dec 6, 2024 •
edited
Loading

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Vitaly Buka (vitalybuka)

Changes

This fix the case, when single hot inlined callsite, prevent
checks for all other. This helps to reduce number of removed checks up to 50% (deppedes on cutoff-hot value) .

ScalarOptimizerLateEPCallback was happening during
CGSCC walk, after each inlining, but this is effectively
after inlining.

Example, order in comments:

static void overflow() {
  // 1. Inline get/set if possible
  // 2. Simplify
  // 3. LowerAllowCheckPass
  set(get() + get());
}

void test() {
  // 4. Inline
  // 5. Nothing for LowerAllowCheckPass
  overflow();
}

With this patch it will look like:

static void overflow() {
  // 1. Inline get/set if possible
  // 2. Simplify
  set(get() + get());
}

void test() {
  // 3. Inline
  // 4. Simplify
  overflow();
}

// Later, after inliner CGSCC walk complete:
// 5. LowerAllowCheckPass for `overflow`
// 6. LowerAllowCheckPass for `test`

Full diff: https://github.com/llvm/llvm-project/pull/119013.diff

2 Files Affected:

  • (modified) clang/lib/CodeGen/BackendUtil.cpp (+6-7)
  • (modified) clang/test/CodeGen/allow-ubsan-check-inline.c (+2-2)
diff --git a/clang/lib/CodeGen/BackendUtil.cpp b/clang/lib/CodeGen/BackendUtil.cpp
index bf9b04f02e9f44..1f0e760e8ebd29 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -789,13 +789,12 @@ static void addSanitizers(const Triple &TargetTriple,
   }
 
   if (LowerAllowCheckPass::IsRequested()) {
-    // We can optimize after inliner, and PGO profile matching. The hook below
-    // is called at the end `buildFunctionSimplificationPipeline`, which called
-    // from `buildInlinerPipeline`, which called after profile matching.
-    PB.registerScalarOptimizerLateEPCallback(
-        [](FunctionPassManager &FPM, OptimizationLevel Level) {
-          FPM.addPass(LowerAllowCheckPass());
-        });
+    // We want to call it after inline, which is about OptimizerEarlyEPCallback.
+    PB.registerOptimizerEarlyEPCallback([](ModulePassManager &MPM,
+                                           OptimizationLevel Level,
+                                           ThinOrFullLTOPhase Phase) {
+      MPM.addPass(createModuleToFunctionPassAdaptor(LowerAllowCheckPass()));
+    });
   }
 }
 
diff --git a/clang/test/CodeGen/allow-ubsan-check-inline.c b/clang/test/CodeGen/allow-ubsan-check-inline.c
index cabe76d8034d77..1de24ab90dac0e 100644
--- a/clang/test/CodeGen/allow-ubsan-check-inline.c
+++ b/clang/test/CodeGen/allow-ubsan-check-inline.c
@@ -7,8 +7,8 @@ void set(int x);
 // We will only make decision in the `overflow` function.
 // NOINL-COUNT-1: remark: Allowed check:
 
-// FIXME: We will make decision on every inline.
-// INLINE-COUNT-1: remark: Allowed check:
+// We will make decision on every inline.
+// INLINE-COUNT-5: remark: Allowed check:
 
 static void overflow() {
   set(get() + get());

@vitalybuka vitalybuka changed the base branch from users/vitalybuka/spr/main.ubsan-improve-lowering-of-llvmallowubsancheck to main December 7, 2024 22:50
@vitalybuka
rebase
84fd27e 
Created using spr 1.3.4
@vitalybuka vitalybuka merged commit 7787328 into main Dec 8, 2024
8 checks passed
@vitalybuka vitalybuka deleted the users/vitalybuka/spr/ubsan-improve-lowering-of-llvmallowubsancheck branch December 8, 2024 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kstoimenov kstoimenov kstoimenov approved these changes

Assignees

No one assigned

Labels

clang:codegen IR generation bugs: mangling, exceptions, etc. clang Clang issues not falling into any other category

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vitalybuka @llvmbot @kstoimenov